feat: Bcrypt algorithm support #1169
Conversation
|
|
seremenko-wish
changed the title
There was a problem hiding this comment.
Thank you for working on this, it looks good! One thing we need to take care of however is that we can't change the algorithm if we have users signed up already, as those users would no longer be able to log in.
This means that we need a password upgrade here: https:/ory/kratos/blob/master/selfservice/strategy/password/login.go#L149
The code for generating the password entry can be found at https:/ory/kratos/blob/master/selfservice/strategy/password/registration.go#L177-L196
It would probably also make sense to add a section about bcrypt here: https:/ory/kratos/blob/master/docs/docs/concepts/credentials/username-email-password.mdx
And lastly, it's actually fine by me if we use BCrypt per default, as the kratos-typical workloads are web based, meaning that Argon2 might underperform.
Co-authored-by: hackerman <[email protected]>
Co-authored-by: hackerman <[email protected]>
Co-authored-by: Patrik <[email protected]>
|
Last commit fixes #1175 |
I separated the hash comparator from hashers, so changing of hashing algorithm won't prevent users from logging in.
fixed
this code does not need any changes as it uses the hasher specified in the configuration
added
For backwards compatibility, I left Argon2id as the default hasher. We can create a separate task to make that switch in the future. |
hash/hasher_bcrypt.go
Outdated
| // so if password is longer than 72 bytes, function returns an error | ||
| // See https://en.wikipedia.org/wiki/Bcrypt#User_input | ||
| if len(password) > 72 { | ||
| return errors.New("passwords are limited to a maximum length of 72 characters") |
There was a problem hiding this comment.
This error will be passed down to the UI I guess. Please add it to the messages to allow translations and mapping to other text: https:/ory/kratos/blob/1940a679eb6b695e22cb939fb0d8d85cebb82a1e/text/message_registration.go
I guess you would then in the flow check for this error you return here similar to
But maybe @aeneasr can give better pointers.
There was a problem hiding this comment.
Right, it would be an error like this one (actually we can just add it do that file for now): https:/ory/kratos/blob/master/schema/errors.go#L59-L70
hash/hasher_bcrypt.go
Outdated
| // so if password is longer than 72 bytes, function returns an error | ||
| // See https://en.wikipedia.org/wiki/Bcrypt#User_input | ||
| if len(password) > 72 { | ||
| return errors.New("passwords are limited to a maximum length of 72 characters") |
There was a problem hiding this comment.
Right, it would be an error like this one (actually we can just add it do that file for now): https:/ory/kratos/blob/master/schema/errors.go#L59-L70
|
Ok. Pushed an update! Pls take a look |
|
The last thing is getting tests green - so running |
|
I have resolved the e2e tests so this should work again - rerunning tests now. |
|
ok. all checks are green now! |
|
Awesome, thank you so much for your hard work! |
|
@aeneasr would you want to add this feature to the next version release? |
|
Yes, this is planned for 0.6 I am currently working on pushing out 0.6 this month! |
|
Looks like OWASP is also not quite clear themselves as to which algorithm to support as they have recently updated their docs to encourage use of Argon2id whenever possible: https://cheatsheetseries.owasp.org/cheatsheets/Password_Storage_Cheat_Sheet.html#pre-hashing-passwords |
|
Yeah. That's fine. I saw that they recommend some argon2 settings. It would be interesting to measure performance with those settings. btw, they encourage not to use any pre-hashing functions with bcrypt https://cheatsheetseries.owasp.org/cheatsheets/Password_Storage_Cheat_Sheet.html#pre-hashing-passwords |
|
Yes, thank you! I also saw that in a talk someone mentioned in another issue :) |
Related issue
#1137 @aeneasr
Proposed changes
hashers.algorithmoption in config to control password hashing algorithm. By default, the value isargon2. In order to use the Bcrypt hashing algorithm,bcryptvalue should be used. Also, added a setting to control the cost for Bcrypt algorithm (hashers.bcrypt.cost) with default value12.Checklist
vulnerability. If this pull request addresses a security. vulnerability, I
confirm that I got green light (please contact
[email protected]) from the maintainers to push
the changes.
works.
Further comments
hashers.algorithmshould be considered a breaking change since passwords hashed by Argon2 won't be properly validated after this change. In order to handle this case properly, hashers should be separated from classes that check hashes. Should we create a separate issue for this?